Preserve file encoding in file-based app source files#52055
Conversation
- Modified SourceFile.Load() to detect UTF-8 BOM in files - Modified SourceFile.Save() to preserve original BOM state - Added HasUtf8Bom property to SourceFile struct - Updated InternalAPI.Unshipped.txt with new API members - Added tests to verify BOM preservation (no-BOM and with-BOM cases) Fixes issue where dotnet add package command was adding BOM to files that originally didn't have one, breaking shebang scripts. Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
- Use SourceText's Encoding property instead of custom HasUtf8Bom tracking - Use stackalloc for 3-byte buffer on .NET Core (with fallback for netstandard2.0) - Update tests to use _testAssetsManager.CreateTestDirectory() infrastructure - Add issue reference (#52054) to test docs - Remove unnecessary API surface from InternalAPI.Unshipped.txt Co-authored-by: jjonescz <3669664+jjonescz@users.noreply.github.com>
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
- Remove custom DetectEncoding() method and use SourceText.From(stream, encoding: null) - SourceText automatically detects encoding including BOM when encoding is null - Move <see href/> tags inside <summary> elements for better documentation - Simplifies implementation while maintaining full functionality Co-authored-by: jjonescz <3669664+jjonescz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where dotnet add file.cs package <package> was inadvertently adding a UTF-8 BOM to file-based app source files, breaking shebang (#!) scripts on Unix-like systems. The fix preserves the original encoding state (with or without BOM) by leveraging Roslyn's SourceText encoding detection and preservation capabilities.
Key Changes:
- Modified
SourceFile.Load()to auto-detect encoding by passingencoding: nulltoSourceText.From() - Updated
SourceFile.Save()to use the detected encoding fromSourceText.Encodingproperty - Added comprehensive tests verifying BOM preservation for both cases (with and without BOM)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs | Updated SourceFile.Load() to auto-detect encoding and SourceFile.Save() to preserve it using SourceText.Encoding |
| test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs | Added two test cases: PreservesNoBomEncoding() for files without BOM and PreservesBomEncoding() for files with BOM |
- Add PreservesNonUtf8Encoding test to verify UTF-16 encoding is preserved - Tests that files with UTF-16 LE encoding (with BOM 0xFF 0xFE) maintain their encoding - Ensures SourceText's auto-detection works for encodings beyond UTF-8 - All encoding preservation tests now pass (UTF-8 no BOM, UTF-8 with BOM, UTF-16) Co-authored-by: jjonescz <3669664+jjonescz@users.noreply.github.com>
333fred
left a comment
There was a problem hiding this comment.
A few minor comments on the tests.
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Fred Silberberg <frsilb@microsoft.com>
|
@333fred @RikkiGibson for another look, thanks |
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM, comment is not blocking.
- Replace Assert.StartsWith with Assert.Equal to verify the entire file content - Ensures the test validates that Save() correctly writes the complete content - Makes the test more robust by checking the full output including the added package directive Co-authored-by: jjonescz <3669664+jjonescz@users.noreply.github.com>
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs
Outdated
Show resolved
Hide resolved
- Replace raw string literal with explicit \n characters in expected content - Raw string literals with indentation were causing blank lines to be stripped - Explicit newlines ensure consistent comparison across platforms - Fixes test failure on Windows where line endings differ Co-authored-by: jjonescz <3669664+jjonescz@users.noreply.github.com>
|
/backport to release/10.0.2xx |
|
Started backporting to |
SourceFile.Save()usesEncoding.UTF8which adds BOM, breaking shebang linesFileLevelDirectiveHelpers.csFileBasedAppSourceEditorTests.csSourceFilestruct to preserve encoding using SourceText.Encoding propertystackallocfor 3-byte BOM detection buffer (with netstandard2.0 fallback)_testAssetsManager.CreateTestDirectory()infrastructure<see href/>inside<summary>elementsencoding: null)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.